Skip to content

Fixed rename by using put instead of post#2764

Merged
bfops merged 14 commits into
masterfrom
tyler/fix-rename
Jun 12, 2025
Merged

Fixed rename by using put instead of post#2764
bfops merged 14 commits into
masterfrom
tyler/fix-rename

Conversation

@cloutiertyler
Copy link
Copy Markdown
Contributor

@cloutiertyler cloutiertyler commented May 20, 2025 •
edited by bfops
Loading

Description of Changes

Closes #2763

API and ABI breaking changes

Technically a breaking change because it changes the behavior of the CLI, but it's arguably a bug fix.

Expected complexity level and risk

2 because of the breaking nature

Testing

  • Test that you can rename a database (I have not done this yet)
    • Confirmed that old db name no longer existed after renaming
  • Existing name-setting smoketest passes

bfops
bfops reviewed May 21, 2025
View reviewed changes
Comment thread crates/cli/src/subcommands/dns.rs Outdated
@bfops
Copy link
Copy Markdown
Collaborator

bfops commented May 21, 2025

@cloutiertyler I updated the code meaningfully, as well as augmenting the smoketest. The new endpoint just returns a string that's either "Success" or an error message, so I've removed all the code that tries to parse a well-typed error out of the response.

cloutiertyler
cloutiertyler commented May 21, 2025
View reviewed changes
Comment thread crates/cli/src/subcommands/dns.rs Outdated
@bfops bfops added release-any To be landed in any release window bugfix Fixes something that was expected to work differently labels May 27, 2025
@bfops
Copy link
Copy Markdown
Collaborator

bfops commented May 27, 2025

(status here: the smoketests have uncovered that the PUT version of the request will happily clobber existing names without emitting an error)

@bfops
Copy link
Copy Markdown
Collaborator

bfops commented Jun 5, 2025

To clarify, you cannot clobber a database that you do not own:

$ cargo run -pspacetimedb-cli -- rename --to rename-test-module rename-from-module
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.35s
     Running `target/debug/spacetimedb-cli rename --to rename-test-module rename-from-module`
Error: Error: {"PermissionDenied":{"domain":"rename-test-module"}}

jdetter
jdetter approved these changes Jun 6, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jdetter jdetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving but I made some of the changes so @bfops should take a look as well before merging.

@jdetter jdetter mentioned this pull request Jun 6, 2025
Closed
bfops
bfops approved these changes Jun 11, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Idk why I had to change it to parse a non-JSON response at one point, but it does seem to consistently return JSON responses now, so.. 🤷

@bfops bfops added this pull request to the merge queue Jun 11, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Jun 11, 2025
@bfops bfops added this pull request to the merge queue Jun 12, 2025
Merged via the queue into master with commit 013e268 Jun 12, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bfops bfops bfops approved these changes

@jdetter jdetter jdetter approved these changes

Assignees

No one assigned

Labels

bugfix Fixes something that was expected to work differently release-any To be landed in any release window

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CLI - rename should remove the old name

3 participants

@cloutiertyler @bfops @jdetter